Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

アウトライン解析ウィンドウのツリービューの設定処理の高速化 #1648

Merged
merged 3 commits into from
May 1, 2021

Conversation

beru
Copy link
Contributor

@beru beru commented Apr 27, 2021

PR の目的

アウトライン解析ウィンドウのツリービューの表示を高速化するのが目的です。

動作確認には #1398 の説明に添付されている test.zip の中の test_100k.txt ファイルを使用しました。

カテゴリ

  • 速度向上

PR の背景

test_100k.txt ファイルを開いた後にF11キーを押してアウトライン解析を行うと、自分が今使っているPCだと表示されるまでに5秒近くかかる事が分かりました。処理時間が掛かりすぎだと思うので高速化しました。

PR のメリット

アウトライン解析の表示が速くなります。

PR のデメリット (トレードオフとかあれば)

CDlgFuncList::SetTreeJavaCDlgFuncList::SetTree の引数を増やしたのでその分コードが少し読みづらくなったかもしれません。

仕様・動作説明

アウトライン解析のツリービューにアイテムを追加する処理の前後で WM_SETREDRAW メッセージを呼ぶようにしました。

表示順が「デフォルト」もしくは「デフォルト(降順)」の場合は、ツリービューにアイテムを追加する際にパラメータ HTREEITEM::hInsertAfter の値を調整する事で、後でソート処理(CDlgFuncList::SortTree)を呼ぶ必要が無くせるのではないかと考えました。ツリービューのソート処理は要素数が多いとその分だけ時間が掛かるので、呼び出しを省くことでその分の処理時間を減らしました。

PR の影響範囲

アウトライン解析ウィンドウの表示処理

テスト内容

テスト1

手順

  • ファイル、例えば test_100k.txt を開く
  • F11キーを押してアウトライン解析を行う
  • 順序を切り替えて表示が変わる事を確認する
  • 再度F11キーを押してアウトライン解析が再度行われることを確認する

関連 issue, PR

#1398

CDlgFuncList::SetTree においてアイテムを追加するループの前後で WM_SETREDRAW メッセージを使用する事で描画の抑制
ソート種別が SORTTYPE_DEFAULT もしくは SORTTYPE_DEFAULT_DESC の場合は、ツリービューへのデータ追加方法を調整する事で CDlgFuncList::SortTree の呼び出しを行う必要が無くして高速化
@beru beru added the 🚅 speed up 🚀 高速化 label Apr 27, 2021
@AppVeyorBot
Copy link

Build sakura 1.0.3713 completed (commit e20736c2b3 by @beru)

@berryzplus
Copy link
Contributor

PR のデメリット (トレードオフとかあれば)

CDlgFuncList::SetTreeJavaCDlgFuncList::SetTree の引数を増やしたのでその分コードが少し読みづらくなったかもしれません。

Java専用のアウトライン表示コードが存在していて、そこに対策します。
と言っているように見えます。

アクションから画面表示までの間に行っている処理が「適切」なのであれば、
どんなに時間がかかったとしても「仕様です。諦めてください。」と言えるように思います。

要望として「一部処理を省略してでも表示を速くしたい」はあるのかも知れません。
そういう場合には仕様変更して「高速モード」を作るんだと思います。

コードをざっと見た感じ、「処理順が不適切」という詳細設計的なバグに対処している雰囲気を感じます。

  • 処理対象のツリーハンドルを取得するには、やや時間がかかる。(WindowsAPIの制約)
  • ツリー表示処理では、処理対象のツリーハンドルが必要。(WindowsAPIの制約)
  • ツリー表示処理に、処理対象のツリーハンドルを渡していない。(アプリの詳細設計ミス)

だとするならば、修正は「高速化したい」から行うべきなんではなくて、「詳細設計が不適切なのは改善すべき」だから行う感じになるような。

個人的には生のツリーハンドルをメソッド引数に採用するのがちょっと嫌です。

あとまぁ、「5秒近くかかる」をどうしても「問題」としたい場合、修正を適用したら何秒まで縮められるかを書くべきと思います。

@beru
Copy link
Contributor Author

beru commented Apr 29, 2021

Java専用のアウトライン表示コードが存在していて、そこに対策します。
と言っているように見えます。

CDlgFuncList::SetTreeJava メソッドですがJava専用というわけではないです。
どうしてこういう命名なのかはよくわかりませんが…。

アクションから画面表示までの間に行っている処理が「適切」なのであれば、
どんなに時間がかかったとしても「仕様です。諦めてください。」と言えるように思います。

その論の、処理が「適切」かどうかを誰がどう決めるのか良くわかりません…。

単純に待ち時間が長いのは嫌なので処理時間が短くなるようにしたいです。

要望として「一部処理を省略してでも表示を速くしたい」はあるのかも知れません。
そういう場合には仕様変更して「高速モード」を作るんだと思います。

今のところ「高速モード」を作る事は考えてません。

動作確認に test_100k.txt ファイルを使いましたが、10万行もあるファイルを開くことはめったにないので、高速化する必要性があるかというとあまり無い気はします。自分はあまりアウトライン解析の速度には困ってません。どちらかというと低機能なのが良くないとは思ってます。

純粋に10万行でアウトライン解析に5秒掛かるのが速いのかそれとも遅いのかでいうと、処理内容をちゃんと見てませんが多分遅い部類だと思います。

コードをざっと見た感じ、「処理順が不適切」という詳細設計的なバグに対処している雰囲気を感じます。

* 処理対象のツリーハンドルを取得するには、やや時間がかかる。(WindowsAPIの制約)

* ツリー表示処理では、処理対象のツリーハンドルが必要。(WindowsAPIの制約)

* ツリー表示処理に、処理対象のツリーハンドルを渡していない。(アプリの詳細設計ミス)

上記の説明が「処理順が不適切」にどう繋がるのかよくわかりませんでした。

berryzplusさんもWindowsAPIを使うので当然分かっていると思いますが、ウィンドウハンドルの取得には殆ど時間は掛からない筈です。該当処理でウィンドウハンドルを取得する関数の呼び出し回数はそもそもそこまで多くないので。

自分がウィンドウハンドルを引数経由で渡すように変更したのは高速化の為ではないです。どちらかというとリファクタリング的な意味合いで変えました。

だとするならば、修正は「高速化したい」から行うべきなんではなくて、「詳細設計が不適切なのは改善すべき」だから行う感じになるような。

とりあえず大幅には変えずに少ない手間で高速化を行おうと考えてます。

個人的には生のツリーハンドルをメソッド引数に採用するのがちょっと嫌です。

ウィンドウハンドルを引数経由で渡すように変更したのは良くなかったですね。
どうしても必要な変更ではないので元のままにしておけば良かったなと思いました。

あとまぁ、「5秒近くかかる」をどうしても「問題」としたい場合、修正を適用したら何秒まで縮められるかを書くべきと思います。

体感では5秒ぐらいかかっていたのが2秒ぐらいになりました。
きちんと処理時間計測した方が良い場合はコード書いて計測します。

@suconbu
Copy link
Member

suconbu commented Apr 29, 2021

10万行もあるファイルを開くことはめったにないので、高速化する必要性があるかというとあまり無い気はします。

個人的には、アウトライン解析表示出しっぱなしのサクラエディタで teraterm のログファイル※ (数千行) をうっかり開いてしまった時のフリーズ時間が短縮されるのが嬉しいですね。
※ログのタイムスタンプ部分 ([Mon Dec 25 09:45:53.530 2017] #) が見出しと認識されてしまうんですよね。。

@beru
Copy link
Contributor Author

beru commented Apr 29, 2021

個人的には、アウトライン解析表示出しっぱなしのサクラエディタで teraterm のログファイル※ (数千行) をうっかり開いてしまった時のフリーズ時間が短縮されるのが嬉しいですね。
※ログのタイムスタンプ部分 ([Mon Dec 25 09:45:53.530 2017] #) が見出しと認識されてしまうんですよね。。

間違えて巨大なファイルを開いてしまってアウトライン解析に時間が掛かる場合はESCキー押しで中断が出来れば良いですね。

@suconbu
Copy link
Member

suconbu commented Apr 29, 2021

処理時間が半分以下になっていたので、どうしてこんなに速くなったのか興味本位で変更を一部戻したりなどして見てみたところ、WM_SETREDRAW を使ってアイテム追加中の再描画をやめた所がほとんどのようでした。
(ソート処理はあってもなくても体感上の時間はほとんど変わりませんでした😅)

旧式なコントロールは描画が遅いということなんでしょうね。。

@beru
Copy link
Contributor Author

beru commented Apr 29, 2021

https://devblogs.microsoft.com/oldnewthing/20110124-00/?p=11683
https://devblogs.microsoft.com/oldnewthing/20040610-00/?p=38933

の解説によると WM_SETREDRAW を使う事で描画だけではなく付随する計算処理の負荷も減るらしいです。

@beru
Copy link
Contributor Author

beru commented Apr 29, 2021

旧式なコントロールは描画が遅いということなんでしょうね。。

標準コントロールの ListView には VirtualMode がありますが TreeView にはないようなので大量のアイテムを扱うのは苦手っぽいですね。

フィルタリング表示は出来ないので、アウトライン解析で大量のアイテムが表示されても操作性が良くない気がします。
ただ一般的には1つのファイルに大量のアウトライン要素を書く使い方はされないと思います。人間が内容を把握しにくくなると思うので。

IDEのクラスビューのように管理している複数のソースコードのファイルのシンボル表示を行うとなると大量のアイテムを表示するので表示パフォーマンスに気を使う必要が出てきそうですね。

@suconbu
Copy link
Member

suconbu commented Apr 29, 2021

の解説によると WM_SETREDRAW を使う事で描画だけではなく付随する計算処理の負荷も減るらしいです。

なるほど。
そういえば、ツリービューは SetData で操作される間は非表示になっていますけど、今回 WM_SETREDRAW が入って高速化したということは、これまで非表示であっても何かしら更新処理は動いてしまっていたということですねえ。

@berryzplus
Copy link
Contributor

Java専用のアウトライン表示コードが存在していて、そこに対策します。
と言っているように見えます。

CDlgFuncList::SetTreeJava メソッドですがJava専用というわけではないです。
どうしてこういう命名なのかはよくわかりませんが…。

関数名CDlgFuncList::SetTreeJavaは不適切である、という見解で良いです?
で、「高速化のためには訂正不要だから対処しません」と言ってるわけですかね?
(それはそれで、別によいような気もしますが、訂正すべきな気がします。reviewの判断はスルーが妥当かな。)

アクションから画面表示までの間に行っている処理が「適切」なのであれば、
どんなに時間がかかったとしても「仕様です。諦めてください。」と言えるように思います。

その論の、処理が「適切」かどうかを誰がどう決めるのか良くわかりません…。

単純に待ち時間が長いのは嫌なので処理時間が短くなるようにしたいです。

画面表示を行うために必要な処理であれば、どんなに時間がかかっても実施するのが筋だと思います。
「不要な処理は実行していない」が「適切」な状態です。

ツリーハンドルを渡し忘れているために再取得が発生するのを対策しているように見えましたが、描画のためにツリーハンドルを「再取得すること」は「必要」なんでしょうか?
(再取得が無駄であることについて、判断のブレは起きないと思います。)

純粋に10万行でアウトライン解析に5秒掛かるのが速いのかそれとも遅いのかでいうと、処理内容をちゃんと見てませんが多分遅い部類だと思います。

主観的なツッコミとしては、
たかだか10万行程度に1秒もかけてんじゃねーよ、
と思います。

上記の説明が「処理順が不適切」にどう繋がるのかよくわかりませんでした。

berryzplusさんもWindowsAPIを使うので当然分かっていると思いますが、ウィンドウハンドルの取得には殆ど時間は掛からない筈です。該当処理でウィンドウハンドルを取得する関数の呼び出し回数はそもそもそこまで多くないので。

自分がウィンドウハンドルを引数経由で渡すように変更したのは高速化の為ではないです。どちらかというとリファクタリング的な意味合いで変えました。

ぼくのコード理解に誤りがありそうな気がしてきました。

が、ウインドウハンドル(hWnd)ではなくて、ツリーハンドルの話をしたつもりでした。

・ルート
 ┣アイテム1
 (省略)
 ┗アイテム10000
  ┗アイテム10000_1

という階層構造を処理するとき「アイテム10000_1」にアクセスするにはツリーハンドルが必要です。
確かに、WindowsAPI関数を利用したハンドル取得はかなり高速ですが、ツリーハンドル取得はSendMessageを介して行う「遅いタイプ」にあたるはずなので、必要と分かってるならサブ処理に渡すほうが効率的です。

だとするならば、修正は「高速化したい」から行うべきなんではなくて、「詳細設計が不適切なのは改善すべき」だから行う感じになるような。

とりあえず大幅には変えずに少ない手間で高速化を行おうと考えてます。

これまでの経緯からすると、単純には「やめといたほうがいい」っすね 😃

あとまぁ、「5秒近くかかる」をどうしても「問題」としたい場合、修正を適用したら何秒まで縮められるかを書くべきと思います。

体感では5秒ぐらいかかっていたのが2秒ぐらいになりました。

2秒・・・・

やっぱり「仕様です。諦めてください」が妥当だと思います。

本当に「仕様」なのか、とか、「遅いんじゃヴォケ!」とかの感想も妥当だと思うので、どっかで対策すべきなのかな。

ある程度時間のかかる処理を「毎回やり直す」ことが、本当に仕様として必要なのか、とか。

@berryzplus
Copy link
Contributor

berryzplus commented Apr 30, 2021

さて、やり取りを読み返してみて、
当初ぼくが書いていたコメントがまったくの大ハズレだったという確信を得ました。

高速化の効果を得られたのは WM_SETREDRAW によるもので、
「描画完了の意図がないツリーアイテム変更に対して発生していた再描画を抑制する」という効果によるものです。

だとすると、
必要のない処理(≒アイテム変更途中の再描画)を除外して適切な状態にしていることになり、
結果として高速化できるっちゅうことになります。

であるなら「SetTreeJavaの変更は余計」ということになり、
紛らわしいから戻しておくのが適切っぽいです。

@suconbu
Copy link
Member

suconbu commented Apr 30, 2021

勝手ながら私の方で SetData 関数の時間内訳の変化を見てみました。

PR説明に書かれている test_100k.txt を一度読み込ませた後、F11キーを2連打して再解析掛けた時のログになります。
※アウトライン解析表示の順序は「デフォルト」
※計測には CRunningTimer というクラスを使いました
※Intel(R) Core(TM) i5-8250U CPU @ 1.60GHz

  1. PR適用前
  0:"SetData" : Enter                    - `SetData` の先頭
  0:"SetData", 17㍉秒 : SW_HIDE          - `::ShowWindow( hwndTree, SW_HIDE )` 直後
  0:"SetData", 2643㍉秒 : DeleteAllItems - `TreeView_DeleteAllItems` 直後
  0:"SetData", 6179㍉秒 : SetTree        - `SetTree` 直後
  0:"SetData", 6264㍉秒 : SetupCombo     - `SortTree` の手前
  0:"SetData", 8777㍉秒 : SortTree       - `SortTree` 直後
  0:"SetData", 8792㍉秒 : Exit Scope     - `SetData` の末尾
  1. PR適用後
  0:"SetData" : Enter 
  0:"SetData", 14㍉秒 : SW_HIDE
  0:"SetData", 2712㍉秒 : DeleteAllItems
  0:"SetData", 4517㍉秒 : SetTree
  0:"SetData", 4534㍉秒 : SetupCombo
  0:"SetData", 4549㍉秒 : SortTree (スキップされている)
  0:"SetData", 4581㍉秒 : Exit Scope

「ソート処理があってもなくても変わらな」かったのは、その時に読み込ませていたデータに依るところがあったようでした。(誤解を招くことを書いてしまいすみません😔)
test_100k.txt のようなデータでは (PR適用前の) SetTree に次ぐ時間が SortTree でも消費されていたようです。

@suconbu
Copy link
Member

suconbu commented Apr 30, 2021

の解説によると WM_SETREDRAW を使う事で描画だけではなく付随する計算処理の負荷も減るらしいです。

なるほど。
そういえば、ツリービューは SetData で操作される間は非表示になっていますけど、今回 WM_SETREDRAW が入って高速化したということは、これまで非表示であっても何かしら更新処理は動いてしまっていたということですねえ。

試しに (ListView|TreeView)_DeleteAllItems の前後にも WM_SETREDRAW を追加したところ該当区間の処理時間が半減してしまいましたので、そういうことだったようです。。

  1. PR適用後 + アイテム全削除中の再描画抑制
  0:"SetData" : Enter 
  0:"SetData", 24㍉秒 : SW_HIDE
  0:"SetData", 1200㍉秒 : DeleteAllItems
  0:"SetData", 2992㍉秒 : SetTree
  0:"SetData", 3009㍉秒 : SetupCombo
  0:"SetData", 3025㍉秒 : SortTree
  0:"SetData", 3058㍉秒 : Exit Scope

@beru
Copy link
Contributor Author

beru commented Apr 30, 2021

関数名CDlgFuncList::SetTreeJavaは不適切である、という見解で良いです?
で、「高速化のためには訂正不要だから対処しません」と言ってるわけですかね?
(それはそれで、別によいような気もしますが、訂正すべきな気がします。reviewの判断はスルーが妥当かな。)

Java以外の言語でも使うわけなので CDlgFuncList::SetTreeJava という名前は不適切だと思います。
とはいえどういう名前に変更すれば良いのか今のところ思いつきません。
このPRでメソッド名を変更する気が無い理由としては、

  • このPRでは高速化を行う事を目的としている為、それ以外の事で悩みたくない
  • SetTree と SetTreeJava の処理内容の違いをきちんと把握できておらず適切な名前を思いつけない
  • 名付けに対してあまり関心が無い

画面表示を行うために必要な処理であれば、どんなに時間がかかっても実施するのが筋だと思います。
「不要な処理は実行していない」が「適切」な状態です。

処理を開始してから完了するまでの時間は世界最速のスパコンを使ってもブラックホールが蒸発するぐらい掛かります、とか言われたら首をしめてしまうと思います。

2秒・・・・

やっぱり「仕様です。諦めてください」が妥当だと思います。

本当に「仕様」なのか、とか、「遅いんじゃヴォケ!」とかの感想も妥当だと思うので、どっかで対策すべきなのかな。

ある程度時間のかかる処理を「毎回やり直す」ことが、本当に仕様として必要なのか、とか。

高速化の為に色々な対策は取れると思います。仕様やデータの持ち方や処理方式を変えたりとか。ただこのPRではそこまでやる事は考えていません。

@beru
Copy link
Contributor Author

beru commented Apr 30, 2021

試しに (ListView|TreeView)_DeleteAllItems の前後にも WM_SETREDRAW を追加したところ該当区間の処理時間が半減してしまいましたので、そういうことだったようです。。

そこでも大きく処理時間が掛かる事があるんですね。手抜きしてちゃんと処理時間の計測をしていなかったので気づきませんでした。

ツリーアイテム群の全削除時のメモリ解放で時間が掛かっているんですかね。。
(ListView|TreeView)_DeleteAllItemsの前後に WM_SETREDRAW を追加してもメモリ解放は行われるのでこれは関係ないですね。全削除でも再描画がこまめに行われてしまうのでしょうか。。

hwndList と hwndTree を非表示にするタイミングの変更
hwndList と hwndTree の描画を WM_SETREDRAW メッセージで抑える範囲を広げる
選択状態更新処理は最後に行うように変更(WM_SETREDRAWメッセージで描画を抑えている間は機能しない模様)
@beru
Copy link
Contributor Author

beru commented Apr 30, 2021

#1648 (comment)@suconbu さんが、(ListView|TreeView)_DeleteAllItems の前後にも WM_SETREDRAW を追加すると処理時間が減るという事を教えてくれたので、WM_SETREDRAW メッセージの呼び出しを SetTree や SetTreeJava メソッド内で行うのではなく呼び出し元の SetData メソッドで行うようにして描画を抑える範囲を広げました。

あと別ブランチCRunningTimer を使って処理時間計測を行いました。
下記が test_100k.txt ファイルを読み込んだ後に F11キーを2回押して Output に出力された処理時間計測結果です。
x64 Releaseビルドを使用しています。

  0:"" : Enter 
  0:"", 15㍉秒 : DeleteAllItems
  0:"", 570㍉秒 : SetTree
  0:"", 848㍉秒 : WM_SETREDRAW
  0:"", 858㍉秒 : Exit Scope
CDialog::OnMove() m_xPos=1517 m_yPos=611
  0:"" : Enter 
  0:"", 548㍉秒 : DeleteAllItems
  0:"", 1102㍉秒 : SetTree
  0:"", 1494㍉秒 : WM_SETREDRAW
  0:"", 1510㍉秒 : Exit Scope

test_100k.txt ファイルのように件数が多いと描画を抑えても全削除に時間が掛かります。
http://vbcity.com/forums/t/26626.aspx によると下記の対処方法があるようです。

  1. 全ての親となるルートのNodeを用意してそれを削除する
  2. 逆順にNodeを削除する
  3. WM_SETREDRAW を使って変更中に描画されるのを防ぐ

今回は3番の方法を使っています。1番は仕様変更になるし2番は実装が大変そうなので諦めます。

@beru
Copy link
Contributor Author

beru commented Apr 30, 2021

変更前の処理時間についても比較用に別ブランチを用意して計測しました。

測定条件は同じで test_100k.txt ファイルを読み込んだ後に F11キーを2回押して Output に出力された処理時間計測結果です。
x64 Releaseビルドを使用しています。

  0:"" : Enter 
  0:"", 15㍉秒 : DeleteAllItems
  0:"", 2036㍉秒 : SetTree
  0:"", 5630㍉秒 : SortTree
  0:"", 5650㍉秒 : Exit Scope
CDialog::OnMove() m_xPos=1517 m_yPos=611
  0:"" : Enter 
  0:"", 2113㍉秒 : DeleteAllItems
  0:"", 4530㍉秒 : SetTree
  0:"", 8247㍉秒 : SortTree
  0:"", 8265㍉秒 : Exit Scope

変更前後の処理時間を並べて比較しました。

  新規表示 再表示
変更前 5650㍉秒 8265㍉秒
変更後 858㍉秒 1510㍉秒

再表示に掛かる処理時間がまだまだ大きくてレスポンスが悪いと思いますが、test_100k.txt ファイルのような大きいファイルを扱う事はめったにないので問題ないかなと思います。

@AppVeyorBot
Copy link

Build sakura 1.0.3717 completed (commit 206407bca1 by @beru)


SetDocLineFuncList();
if( OUTLINE_C_CPP == m_nListType || OUTLINE_CPP == m_nListType ){ /* C++メソッドリスト */
m_nViewType = VIEWTYPE_TREE;
SetTreeJava( GetHwnd(), TRUE ); // Jan. 04, 2002 genta Java Method Treeに統合
SetTreeJava( GetHwnd(), hInsertAfter, TRUE ); // Jan. 04, 2002 genta Java Method Treeに統合
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

結局、引数を増やすんです?

「引数を渡さないのは非効率なのは自明」だけど、実害は出てないと理解しました。

紛らわしいので引数追加はrevert(と同じになるようにコミットを積む)が良さそうに思います。

目下、自分が気にしているのはあとココだけです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

引数を増やさないで同じ処理を行うとなると下記の手段が考えられます。

  1. CDlgFuncList クラスにメンバー変数 HTREEITEM m_hInsertAfter を追加してそれ経由で情報を渡す
  2. SetTreeSetTreeJavam_nSortType の値を元に hInsertAfter ローカル変数を初期化する記述を重複して書く

関連する実装について説明します。

hInsertAfter 引数は SetTreeSetTreeJava の中で呼び出す TreeView_InsertItem マクロの第2引数に渡される TV_INSERTSTRUCT 構造体の hInsertAfter メンバーに設定しています。

元の実装では SetTree では TVI_FIRSTTV_INSERTSTRUCT::hInsertAfter に設定されていました。そして SetTreeJava では TVI_LASTTV_INSERTSTRUCT::hInsertAfter に設定されていました。

このPRの実装では m_nSortType の値によって TVI_FIRSTTVI_LAST のどちらを設定するかを変える事で、後で SortTree の呼び出しを省けようにしています。なお SortTree の呼び出しを省いているのは順序が デフォルトデフォルト(降順) の場合のみです。それらについてはツリービューへの追加順序を調整すれば後でソートしないでも問題ないだろうと判断しました。

https://docs.microsoft.com/en-us/windows/win32/api/commctrl/ns-commctrl-tvinsertstructa

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと分からなかったのですが「PRの実現に必要な変更」なのであれば、そう言って貰えれば良いです。

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

どちらかというと必要な変更だと思ってます。こうする事で SortTree の呼び出しを省いて高速化できるので。

@berryzplus
Copy link
Contributor

ツリーアイテム群の全削除時のメモリ解放で時間が掛かっているんですかね。。
(ListView|TreeView)_DeleteAllItemsの前後に WM_SETREDRAW を追加してもメモリ解放は行われるのでこれは関係ないですね。全削除でも再描画がこまめに行われてしまうのでしょうか。。

挙動から察するに、全削除の場合でも描画のための何かの処理が行われている、ということが言えそうです。

アプリ仕様的に、全削除直後の状態を表示する必要はないので、やっぱりそれも「無駄な処理」にあたるのではないかと思います。(該当の描画処理を抑制して高速化するのは、適切だと思います。)

@beru
Copy link
Contributor Author

beru commented May 1, 2021

アプリ仕様的に、全削除直後の状態を表示する必要はないので、やっぱりそれも「無駄な処理」にあたるのではないかと思います。(該当の描画処理を抑制して高速化するのは、適切だと思います。)

元の実装では CDlgFuncList::SetData() の冒頭で ::ShowWindow( hwndTree, SW_HIDE ); の呼び出しを手前で行う事で、コントロールを非表示にしてから更新を行うようにして、最後に処理が完了した後に ::ShowWindow(hwndShow, SW_SHOW); で表示を行っています。

このPRでは処理の冒頭でコントロールを非表示にはしないようにしました。コントロールを一時的に非表示にするより表示しっぱなしにしておいた方が画面の見た目の変化が自然になる為です。

なお WM_SETREDRAW メッセージで挟む対策でツリービューの描画を抑制をしているんですが、何故かアイテムの全削除やそれに伴うツリービューのスクロールバーの更新等の途中経過が見えてしまいます。本当はそれらは見せたくないんですが対処方法が分からないので諦めました。

@sonarcloud
Copy link

sonarcloud bot commented May 1, 2021

@AppVeyorBot
Copy link

Build sakura 1.0.3718 completed (commit a999ab9b34 by @beru)

Copy link
Contributor

@berryzplus berryzplus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと全容が理解できていないですが、良さそうに思います。

@beru
Copy link
Contributor Author

beru commented May 1, 2021

レビューありがとうございます。マージします。
もし不具合が見つかったら別PRで修正します。

@beru beru merged commit b68f544 into sakura-editor:master May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants